Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add a way to view TLS certificates when error #2428

Merged
merged 1 commit into from
Jul 19, 2016
Merged

Add a way to view TLS certificates when error #2428

merged 1 commit into from
Jul 19, 2016

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jul 9, 2016 •
edited by diracdeltas
Loading

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Ran git rebase -i to squash commits if needed.

Fix #1057

Problems needs to be solved:

<img width="257" alt="screen shot 2016-07-09 at 16 42 07" src="https://cloud.githubusercontent.com/assets/11330831/16707020/75a32432-45f4-11e6-8425-96f5c442e4de.png">~~~

~~~\- [ ] Waiting for [`UMD` problem](https://github.com/digitalbazaar/forge/issues/198) of [node-forge](https://github.com/digitalbazaar/forge) to be [fixed](https://github.com/digitalbazaar/forge/pull/357) then we can use this library to parse certificate info~~~
- [x] Waiting for brave/electron#25 being merged
- [x] Waiting for brave/electron#27 being merged

@darkdh darkdh added the needs-info Another team member needs information from the PR/issue opener. label Jul 9, 2016
diracdeltas
diracdeltas reviewed Jul 11, 2016
View reviewed changes
package.json
@@ -77,6 +77,7 @@
"keytar": "^3.0.0",
"l20n": "^3.5.1",
"lru_cache": "^1.0.0",
"node-forge": "^0.6.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be taken out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM, just saw your comment

Copy link
Member Author

@darkdh darkdh Jul 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some little hack on electron for 77c534a. I think maybe we can make some changes on electron to extract the detail attributes from certificate. It may be a long term and better solution and we can reduce the effect to integrate with node-forge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems good for now

@diracdeltas
Copy link
Member

great start, thanks

@darkdh darkdh mentioned this pull request Jul 12, 2016
Merged
@darkdh
Copy link
Member Author

darkdh commented Jul 12, 2016

The screenshot of 77c534a
screen shot 2016-07-13 at 01 11 14

@diracdeltas diracdeltas added upstream and removed needs-info Another team member needs information from the PR/issue opener. labels Jul 12, 2016
@darkdh
Copy link
Member Author

darkdh commented Jul 13, 2016

The value of Serial Number we show is not correct. I'm working on it.

@darkdh darkdh force-pushed the 1057 branch 2 times, most recently from 1d3764e to 4ae87f8 Compare July 15, 2016 18:26
@darkdh
Copy link
Member Author

darkdh commented Jul 15, 2016 •
edited
Loading

The serial number issue has been fixed with brave/muon#27
CC @diracdeltas , @bbondy
screen shot 2016-07-16 at 01 55 26

@diracdeltas
Copy link
Member

Is the serialnumber fix in the latest electron release?

@bbondy
Copy link
Member

bbondy commented Jul 18, 2016

I'll be doing new builds son, maybe today.

@bbondy
Copy link
Member

bbondy commented Jul 19, 2016

It is available with npm install now and will be in beta3

@diracdeltas
Copy link
Member

++, thanks

@diracdeltas diracdeltas merged commit 698ec77 into brave:master Jul 19, 2016
@diracdeltas
Copy link
Member

Actually there is a problem where the serial number is undefined if it's above a certain length. Ex: https://expired.badssl.com. will push a fix to Electron

@darkdh
Copy link
Member Author

darkdh commented Jul 20, 2016 •
edited
Loading

This PR(brave/muon#28) will fixed the issue you described. I just tested it with the commit and it works fine.

screen shot 2016-07-20 at 13 41 21

@luixxiul luixxiul added this to the 0.11.2dev milestone Aug 2, 2016
@darkdh darkdh deleted the 1057 branch August 4, 2016 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@diracdeltas diracdeltas

Labels
upstream
Projects
None yet
Milestone
0.11.2dev
Development

Successfully merging this pull request may close these issues.

add a way to view TLS certificates
4 participants
@darkdh @diracdeltas @bbondy @luixxiul